-
-
Notifications
You must be signed in to change notification settings - Fork 230
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
chore: setvar minor fix, tests, added warning when missing variable, deprecates usage of tx.LogData #892
Conversation
Codecov ReportAttention:
📢 Thoughts on this report? Let us know!. |
internal/actions/setvar_test.go
Outdated
var invalidSyntaxAtoiError = "invalid syntax" | ||
var warningKeyNotFoundInCollection = "key not found in collection" | ||
|
||
func TestSetvarEvaluateErrors(t *testing.T) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IIUC, there is a fix related to -
should there be a new test for it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, Numerical operation - with existing negative variable
and Numerical operation - with existing variable
are the two test cases added to this new test to ensure that the fix related to -
is working as expected.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah I think I got confused by the test function name, it looks like not all of these are errors
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, sorry about that. The test evolved over time and is not anymore just about errors. Renamed it to TestSetvarEvaluate
This PR:
+
and-
operations.setvar:TX.score=+5
: even ifTX.score
does not exist, it defaults to 0 and the variable is initialized to5
.setvar:TX.score=-5
: was failing. Now, the behavior is aligned. Even ifTX.score
does not exist, it defaults to 0 and the variable is initialized to-5
.Evaluate
performs the macro expansion ahead of time (when iterating over all the not disruptive actions) and stores the result in the (to my eyes) unusedtx.LogData
variable. This PR proposes to:Evaluate
a no op. The expansion is already performed after all the other actions (and therefore after the expected variable has been populated).tx.LogData
. As far as I can see it was populated only there, and never used. LogData expanded values are indeed stored inside theMatchData struct
and saved inside the transaction intx.matchVariable
. If the reasoning is correct, it should also lead to tiny performance improvements, avoiding useless macro expansion attempts. A double-check about it would be very welcome.Closes #888